Skip to content

Conversation

@mo-philrelton
Copy link
Contributor

EPPT-2588

In order to calculate the daily Fire Severity Index, we require a Buildup Index calculation. This class, and associated tests, partially reproduce the Canadian Forest Fire Weather Index from van Wagner and Pickett's 1985 FORTRAN implementation

Testing:

  • Ran tests and they passed OK
  • Added new tests for the new feature(s)

@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (EPPT_2411_fire_severity_index_workflow_development@a2fcead). Learn more about missing BASE report.

Additional details and impacted files
@@                                  Coverage Diff                                  @@
##             EPPT_2411_fire_severity_index_workflow_development    #2253   +/-   ##
=====================================================================================
  Coverage                                                      ?   95.25%           
=====================================================================================
  Files                                                         ?      152           
  Lines                                                         ?    15369           
  Branches                                                      ?        0           
=====================================================================================
  Hits                                                          ?    14640           
  Misses                                                        ?      729           
  Partials                                                      ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mo-philrelton mo-philrelton changed the base branch from master to EPPT_2411_fire_severity_index_workflow_development December 1, 2025 10:23
@Anzerkhan27 Anzerkhan27 self-requested a review December 19, 2025 16:48
Copy link
Contributor

@Anzerkhan27 Anzerkhan27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation is consistent with the existing fire weather plugins, the structure aligns with the FireWeatherIndexBase. The tests provide good coverage

Copy link
Member

@MoseleyS MoseleyS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've picked up on a few things to think about. The main thing is that I don't think you should include tests of base-class functions in the concrete-class tests.

Comment on lines 15 to 17
def input_cubes(
dmc_val: float = 10.0,
dc_val: float = 15.0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could use pytest fixtures to simplify these tests:

Suggested change
def input_cubes(
dmc_val: float = 10.0,
dc_val: float = 15.0,
@pytest.fixture
def input_cubes(
dmc_val: float,
dc_val: float,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of using the fixtures. I think it's easier to read as a called function, and means you don't have to pass it as a parameter to each test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like fixtures, but they don't offer much benefit here, so I won't push it.

What I did spot is that the default values for input_cubes() are never used, which was what put fixtures in my mind as they do play nicely with pytest.mark.parametrize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants